Use local_info.node() instead of bootstrap.node() whenever possible by dio · Pull Request #4120 · envoyproxy/envoy

dio · 2018-08-13T00:28:05Z

Description:
Use local_info.node() instead of bootstrap.node() whenever possible.

Risk Level: Low
Testing: Existing
Docs Changes: N/A
Release Notes: N/A
Fixes #4085

Signed-off-by: Dhi Aurrahman dio@tetrate.io

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123 · 2018-08-13T02:30:11Z

Test please?

dio · 2018-08-13T03:10:57Z

@mattklein123 yes, sorry missing some commits. I'm on it.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

ramaraochavali · 2018-08-13T04:04:37Z

WDYT about adding an ASSERT while creating DiscoveryRequest which validates the existence of these two fields in the request?

dio · 2018-08-13T04:09:37Z

@mattklein123 I feel no confidence on the change for initializing LoadStatsReporter here: https://github.com/envoyproxy/envoy/pull/4120/files#diff-982df999d2cabd23afd6c25088035bf5R304 getting node() information from local_info instead of bootstrap; this is for consistency reason only (while LoadStatsReporter doesn't use id() or cluster() value).

And I haven't found a way to check the effect of those CLI flags directly. But, since it is being forced for xDS, via Utility::checkLocalInfo, I think the change here is fine.

dio · 2018-08-13T04:11:15Z

@ramaraochavali yeah, I'm thinking the same, what do you think about 6c263d3? Do you have any suggestions to add similar checks in other places?

ramaraochavali · 2018-08-13T10:27:57Z

source/common/upstream/cluster_manager_impl.cc

  if (bootstrap.dynamic_resources().has_ads_config()) {
    ads_mux_.reset(new Config::GrpcMuxImpl(
-        bootstrap.node(),
+        local_info.node(),


Can we add method similar to Config::Utility::checkLocalInfo("ads", local_info)
https://github.com/envoyproxy/envoy/blob/master/source/common/config/grpc_mux_impl.cc#L18
and validate that node has right info?

Yes, we can. However currently, if we failed to supply the info, it firstly throws an error for "cds", which I think serves the same purpose.

[2018-08-13 17:32:35.821][2350767][critical][main] source/server/server.cc:78] error initializing configuration '/envoy/config-local.yaml': cds: setting --service-cluster and --service-node is required

@ramaraochavali after an experiment, I think it is tempting to have Config::Utility::checkLocalInfo inside the GrpcMuxImpl's constructor (and testing it is easier, yeay!). However, since we want to pass in the local_info here, the implication is quite large (but mostly "replacing" node with local_info). I have the changeset ready but want to gather opinions about it.

cc. @htuch @mattklein123.

ramaraochavali · 2018-08-13T10:32:16Z

test/integration/ads_integration_test.cc

    envoy::api::v2::DiscoveryRequest discovery_request;
    VERIFY_ASSERTION(ads_stream_->waitForGrpcMessage(*dispatcher_, discovery_request));

+    ASSERT(discovery_request.has_node());


I think while these ASSERTs are good, they would just validate existing tests right? Can we add a test in grpc_mux_impl_test that validates bad Node passed to it along the lines of this test

envoy/test/server/lds_api_test.cc

Line 203 in 0228180

TEST_F(LdsApiTest, BadLocalInfo) {

Thanks for this. Will try to read and add a test.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch · 2018-08-14T13:41:26Z

test/integration/load_stats_integration_test.cc

      return;
    } else if (loadstats_request.cluster_stats_size() == 0) {
      loadstats_request.CopyFrom(local_loadstats_request);
+      ASSERT(loadstats_request.has_node());


You probably want ASSERT_TRUE to use the asserts from gtest.

htuch · 2018-08-14T13:42:45Z

source/common/upstream/cluster_manager_impl.cc

    const auto& load_stats_config = cm_config.load_stats_config();
    load_stats_reporter_.reset(
-        new LoadStatsReporter(bootstrap.node(), *this, stats,
+        new LoadStatsReporter(local_info.node(), *this, stats,


Another option, not saying you have to do this, is to treat the CLI flags as a bootstrap override and override the bootstrap proto early in server config, so that local_info and bootstrap are consistent. Or, maybe nuke node from bootstrap once you've loaded up local_info. Ideally we have a single source-of-truth, and it's hard to confuse with other potential sources.

+1, it would be great to have a single source of truth here. From an interface perspective it seems better to have LocalInfo be the source of truth but I don't feel that strongly about it.

Updated to use local_info.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

ramaraochavali · 2018-08-15T04:20:45Z

@dio Thanks for this. I like the validation in the constructor. Tests looks much cleaner. LGTM

htuch · 2018-08-15T19:58:41Z

test/common/config/grpc_mux_impl_test.cc

+          *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
+              "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"),
+          random_, time_source_),
+      EnvoyException, "ads: setting --service-cluster and --service-node is required");


Unrelated: these error messages are misleading nowadays, since you can specify this same information in the bootstrap.

I had some difficulties with crafting related test before, so I guess I need to read more on bootstrap initialization steps (server init).

Think we should revisit the thrown error message from Config::Utility::checkLocalInfo(_, _);.

Yep, agree.

I have updated the message, not sure about its clarity and correctness.

htuch · 2018-08-15T19:59:02Z

test/common/config/grpc_mux_impl_test.cc

+  void setup() {
    EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) {
      timer_cb_ = timer_cb;
+      timer_ = new Event::MockTimer();


Unrelated change?

Since the newly introduced tests do not call setup(), and timer_ was previously initialized as new Event::MockTimer() at constructor, this makes sure no leak. Caught by LeakSanitizer in this build: https://circleci.com/gh/envoyproxy/envoy/84216.

But yeah if we go with the other direction it will be reverted.

htuch · 2018-08-15T20:00:43Z

source/common/upstream/cluster_manager_impl.cc

  if (bootstrap.dynamic_resources().has_ads_config()) {
    ads_mux_.reset(new Config::GrpcMuxImpl(
-        bootstrap.node(),
+        local_info,


I have mixed opinions on this change. On the one hand, we make it more full proof, by forcing the LocalInfo type. On the other, we are now passing a strict superset of what we were passing before, and the new information is not used. My main thoughts on making LocalInfo canonical would be to just delete the node info from bootstrap very early in server init. But, this approach also works..

Interesting POV, will try to explore that too.

It's not necessarily the correct PoV; just sharing where my thinking was originally :)

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch

LGTM, let's ship this as it's a definite improvement, we can bikeshed about internals later on.

* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

Use local_info instead

80b5498

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio mentioned this pull request Aug 13, 2018

--service-node and --service-cluster are not used when sending DiscoveryRequest #4085

Closed

dio changed the title ~~Use local_info.node() instead of bootstrap.node() when possible~~ Use local_info.node() instead of bootstrap.node() whenever possible Aug 13, 2018

mattklein123 self-assigned this Aug 13, 2018

Add tests

6c263d3

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

ramaraochavali reviewed Aug 13, 2018

View reviewed changes

Use local_info for LoadStatsReporter

fd3071e

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch reviewed Aug 14, 2018

View reviewed changes

dio added 2 commits August 15, 2018 03:50

Use local_info everywhere

1792e22

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Make sure timer is owned

0655cf8

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch reviewed Aug 15, 2018

View reviewed changes

mattklein123 assigned htuch Aug 16, 2018

dio added 2 commits August 19, 2018 09:16

Merge remote-tracking branch 'upstream/master' into use-local-info

53c3671

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

review: change the thrown error message

8b0dd55

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch approved these changes Aug 20, 2018

View reviewed changes

htuch merged commit ef937d7 into envoyproxy:master Aug 20, 2018

Conversation

dio commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Aug 13, 2018

Uh oh!

dio commented Aug 13, 2018

Uh oh!

ramaraochavali commented Aug 13, 2018

Uh oh!

dio commented Aug 13, 2018

Uh oh!

dio commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dio commented Aug 13, 2018 •

edited

Loading

dio commented Aug 13, 2018 •

edited

Loading

ramaraochavali Aug 13, 2018 •

edited

Loading

dio Aug 13, 2018 •

edited

Loading

ramaraochavali Aug 13, 2018 •

edited

Loading

dio Aug 13, 2018 •

edited

Loading

dio Aug 15, 2018 •

edited

Loading

dio Aug 15, 2018 •

edited

Loading